test_runner: add mock-timers support for AbortSignal.timeout#60751
test_runner: add mock-timers support for AbortSignal.timeout#60751DeveloperViraj wants to merge 4 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
|
Hi @Renegade334 @ErickWendel 👋 I’ve opened a fresh PR with a clean branch so it’s easier to review. Please let me know if anything needs adjusting, happy to update it! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60751 +/- ##
==========================================
+ Coverage 88.54% 89.62% +1.08%
==========================================
Files 703 706 +3
Lines 208252 219181 +10929
Branches 40153 41988 +1835
==========================================
+ Hits 184391 196441 +12050
+ Misses 15869 14617 -1252
- Partials 7992 8123 +131
🚀 New features to boost your workflow:
|
8cbc185 to
1b2d32f
Compare
|
I've updated both the implementation and the regression test: Fixed the missing function name & trailing comma issues in mock_timers.js Cleaned up unused variables Ensured the mock AbortSignal.timeout() matches Node.js behavior Updated the test file to avoid restricted syntax and comply with Node’s linting rules The PR is ready for the next round of review |
Renegade334
left a comment
There was a problem hiding this comment.
This is looking good! Just a couple of recommended tweaks.
StefanStojanovic
left a comment
There was a problem hiding this comment.
LGTM. Make sure you fix issues found by the linter.
1b2d32f to
ef06137
Compare
|
All requested updates have now been applied. Summary of changes:Updated mock_timers.js according to the reviewer’s feedback Preserved the required comment and applied all suggested code adjustments Updated the test file to follow the Node.js test style (including loading common first) Verified everything locally, all lint issues are resolved @StefanStojanovic @Renegade334 |
|
@Renegade334 |
|
Hi @Renegade334 I’ve addressed the requested changes: Reverted the EnableOptions typedef to the narrower SupportedApis version Updated the AbortSignal.timeout test to use the public mock API from node:test Removed internal imports and the unused variable as suggested Everything should now align with the review feedback. |
|
@DeveloperViraj I think this is very close to landing. One of the items from René #60751 (comment) I consider not a nit. Could you take a look at it? 🙂 |
|
Let's get this over the line. |
Co-authored-by: René <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
1f622a9 to
177ab11
Compare
There was a problem hiding this comment.
| value(delay) { |
There was a problem hiding this comment.
Looks like GH is have a wee stroke with the diffs
There was a problem hiding this comment.
nit: the current looks a bit weird.
| mock.#setTimeout( | |
| () => controller.abort, | |
| delay, | |
| ); |
There was a problem hiding this comment.
Looks like GH is have a wee stroke with the diffs
|
Thank you @Renegade334 for getting this across, and Viraj for the original work. |
Commit Queue failed- Loading data for nodejs/node/pull/60751 ✔ Done loading data for nodejs/node/pull/60751 ----------------------------------- PR info ------------------------------------ Title test_runner: add mock-timers support for AbortSignal.timeout (#60751) Author Viraj Jadhav <[email protected]> (@DeveloperViraj, first-time contributor) Branch DeveloperViraj:fix/mock-timers-abortsignal-timeout-v3 -> nodejs:main Labels semver-minor, needs-ci, commit-queue-squash, test_runner Commits 4 - test_runner: add mock-timers support for AbortSignal.timeout - test_runner: revert EnableOptions typedef change and update AbortSign… - Apply suggestions from code review - remove CRLF introduced by GitHub code review Committers 3 - DeveloperViraj <[email protected]> - GitHub <[email protected]> - Renegade334 <[email protected]> PR-URL: https://github.com/nodejs/node/pull/60751 Fixes: https://github.com/nodejs/node/issues/60509 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]> Reviewed-By: René <[email protected]> Reviewed-By: Jacob Smith <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/60751 Fixes: https://github.com/nodejs/node/issues/60509 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]> Reviewed-By: René <[email protected]> Reviewed-By: Jacob Smith <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Sun, 16 Nov 2025 17:55:20 GMT ✔ Approvals: 4 ✔ - Moshe Atlow (@MoLow): https://github.com/nodejs/node/pull/60751#pullrequestreview-3472027080 ✔ - Stefan Stojanovic (@StefanStojanovic): https://github.com/nodejs/node/pull/60751#pullrequestreview-3472099714 ✔ - René (@Renegade334): https://github.com/nodejs/node/pull/60751#pullrequestreview-4141405569 ✔ - Jacob Smith (@JakobJingleheimer): https://github.com/nodejs/node/pull/60751#pullrequestreview-4157423483 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2026-04-20T18:15:20Z: https://ci.nodejs.org/job/node-test-pull-request/72806/ - Querying data for job/node-test-pull-request/72806/ ✔ Build data downloaded ✔ Last Jenkins CI successful ⚠ PR author is a new contributor: @DeveloperViraj([email protected]) ⚠ - commit 9ffc42dd8217 is authored by [email protected] -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/24799848306 |
PR-URL: #60751 Fixes: #60509 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Stefan Stojanovic <[email protected]> Reviewed-By: René <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
|
Landed in d1ac02f |
Fixes: #60509
This PR completes the fix for making
AbortSignal.timeout()compatible withthe test runner's mock timers, and adds a regression test to ensure correct
behavior going forward.
What’s included
1. Mock timers fix
"AbortSignal.timeout"as a discrete mock-timers API target.AbortSignal.timeoutproperty (no silent failures).toFake/toRealpaths to matchthe design of other mockable timer APIs.
unintentionally removed earlier.
2. New regression test
test/parallel/test-mock-timers-abortsignal-timeout.js{ apis: ['AbortSignal.timeout'] }worksmock.tick()correctly triggers abortion after the expected delayNotes
I created a fresh branch to ensure a clean diff without unrelated changes and
to fully incorporate the feedback from @Renegade334 and @ErickWendel.
Please let me know if you'd like any adjustments to the test or implementation.
Thanks for the clear guidance and review support